all: more linters #24783
all: more linters #24783
Conversation
| @@ -79,7 +79,7 @@ func (n *upnp) ExternalIP() (addr net.IP, err error) { | |||
| func (n *upnp) AddMapping(protocol string, extport, intport int, desc string, lifetime time.Duration) error { | |||
| ip, err := n.internalAddress() | |||
| if err != nil { | |||
| return nil | |||
| return nil // TODO: Shouldn't we return the error? | |||
holiman
requested review from
fjl,
zsfelfoldi,
rjl493456442,
gballet,
s1na,
karalabe and
MariusVanDerWijden
as code owners
.golangci.yml
Outdated
| text: 'SA1019: package github.com/golang/protobuf/proto is deprecated' | ||
| exclude: | ||
| - 'SA1019: event.TypeMux is deprecated: use Feed' | ||
| - 'SA1029: should not use built-in type string as key for value' |
There was a problem hiding this comment.
Where do we get this warning? I thought I had already removed all context string keys.
There was a problem hiding this comment.
It's mainly how I use it to shove some key-values for the clef pipeline
There was a problem hiding this comment.
Ahh, I found it. You need to fix this instead of disabling the warning.
There was a problem hiding this comment.
Yes, I know
That should be fixed at some point, but may require some attention.
I didn't include it in this PR, because it's so large already. For things which require a bit more effort I thought it better to postpone a bit. When changing that, I need to check that it doesn't break the places where we pick those keys out downstream
|
Ok fixing the context can be done here diff --git a/cmd/clef/main.go b/cmd/clef/main.go
index f7c3adebc4..de528982c7 100644
--- a/cmd/clef/main.go
+++ b/cmd/clef/main.go
@@ -819,9 +819,9 @@ func confirm(text string) bool {
func testExternalUI(api *core.SignerAPI) {
- ctx := context.WithValue(context.Background(), "remote", "clef binary")
- ctx = context.WithValue(ctx, "scheme", "in-proc")
- ctx = context.WithValue(ctx, "local", "main")
+ connInfo := rpc.PeerInfo{Transport: "in-proc", RemoteAddr: "localhost"}
+ ctx = context.WithValue(context.Background(), peerInfoContextKey{}, connInfo)
+
errs := make([]string, 0)
a := common.HexToAddress("0xdeadbeef000000000000000000000000deadbeef")
@@ -952,7 +952,11 @@ func testExternalUI(api *core.SignerAPI) {
{ // Metadata
api.UI.ShowInfo("Please check if you see the Origin in next listing (approve or deny)")
time.Sleep(delay)
- api.List(context.WithValue(ctx, "Origin", "origin.com"))
+
+ connInfo := rpc.PeerInfo{Transport: "http"}
+ connInfo.HTTP.Origin = "origin.com"
+
+ api.List(context.WithValue(ctx, peerInfoContextKey{}, connInfo))
expectResponse("metadata - origin", "Did you see origin (origin.com)? [yes/no] ", "yes")
}
However, there are also a couple of these in |
|
Damn, I did not consider unit testing when adding |
|
I now think you are right, we should disable the warning for now and deal with it in another PR. |
Then we should be good to go with this PR |
There was a problem hiding this comment.
LGTM
It does remove a lot of unused code within LES though (that might be used by @zsfelfoldi metering/auctioning bandwith prs) so better to take a look there @zsfelfoldi
|
Wrt the unused stuff, it's a "regression" from a refactor from last year https://github.com/ethereum/go-ethereum/pull/22163/files. I guess @rjl493456442 wantd to split my mega test method into more digestible chunks or variations and some were not necessary at the end. I'm all for double checking and getting rid of all dead code there, just let's make sure nothing was lost in translation. |
core/blockchain_snapshot_test.go
Outdated
| type restartCrashSnapshotTest struct { | ||
| snapshotTestBasic | ||
| newBlocks int | ||
| } | ||
|
|
||
| //nolint:unused |
There was a problem hiding this comment.
@rjl493456442 this comes from https://github.com/ethereum/go-ethereum/pull/22163/files#diff-3ecc1958cb0ed978a0edcf6c43089b130034057420e4ce2e1e0c447cd7cec454R968 -- it's not used, it was originally used in TestRecoverSnapshotFromCrashWithLegacyDiffJournal (which was Skipped) but not is no longer in the codebase. It seems to have been removed here: d6ffa14.
So I'll just remove these structs then.. ?
core/blockchain_sethead_test.go
Outdated
| @@ -44,13 +44,14 @@ type rewindTest struct { | |||
|
|
|||
| setheadBlock uint64 // Block number to set head back to | |||
| expCanonicalBlocks int // Number of canonical blocks expected to remain in the database (excl. genesis) | |||
| expSidechainBlocks int // Number of sidechain blocks expected to remain in the database (excl. genesis) | |||
| expSidechainBlocks int // Number of sidechain blocks expected to remain in theheader = rawdb.ReadHeader(db, hash, i) database (excl. genesis) | |||
| @@ -144,9 +144,9 @@ func benchmarkBloomBits(b *testing.B, sectionSize uint64) { | |||
| db.Close() | |||
| } | |||
|
|
|||
| var bloomBitsPrefix = []byte("bloomBits-") | |||
|
|
|||
| //nolint:unused | |||
There was a problem hiding this comment.
clearBloomBits.
It's used within a test which is Skipped, so technically it's not totally unused, but as far as the linter is concerned, it's not used.
internal/ethapi/api.go
Outdated
| @@ -1546,7 +1546,7 @@ func (s *PublicTransactionPoolAPI) GetRawTransactionByHash(ctx context.Context, | |||
| func (s *PublicTransactionPoolAPI) GetTransactionReceipt(ctx context.Context, hash common.Hash) (map[string]interface{}, error) { | |||
| tx, blockHash, blockNumber, index, err := s.b.GetTransaction(ctx, hash) | |||
| if err != nil { | |||
| return nil, nil | |||
| return nil, err | |||
There was a problem hiding this comment.
Wasn't this meant to mean that "we don't know this tx, so it's not an error, simply return nothing"?
This enables the following linters - typecheck - bidichk - durationcheck - exportloopref WIth a few exceptions. - We use a deprecated protobuf in trezor. I didn't want to mess with that, since I cannot meaningfully test any changes there. - The deprecated TypeMux is used in a few places still, so the warning for it is silenced for now. - Using string type in context.WithValue is apparently wrong, one should use a custom type, to prevent collisions between different places in the hierarchy of callers. That should be fixed at some point, but may require some attention. - The warnings for using weak random generator are squashed, since we use a lot of random without need for cryptographic guarantees. - .golangci.yml file has been modified for WEMIX.(remove unused,staticcheck,gosec,varcheck)
This PR enables the following linters
WIth a few exceptions.
stringtype incontext.WithValueis apparently wrong, one should use a custom type, to prevent collisions between different places in the hierarchy of callers. That should be fixed at some point, but may require some attention.